-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docker test environment updates #2843
Conversation
I really don't understand the coverage reports here. Looking at one of the docker runs, ubuntu-xenial:
|
That coverage report is Python-only and comes before after_success.sh is run, where the C coverage is combined. Combined coverage is meant to be uploaded to Coveralls/Codecov. |
Here's the Codecov report for this commit, which looks more-or-less sensible Python + C coverage: https://codecov.io/gh/python-pillow/Pillow/tree/387ec79cebad1be4fe8670200032afea3ae20402 |
I did notice a drop in coverage today: https://codecov.io/gh/python-pillow/Pillow/commits PR #2841 did nothing to change coverage (just some lines a test changed), and the PR build itself reported no change in coverage. So what changed? The "Make and test with coverage" PR was merged in the interim: python-pillow/docker-images#12.
So tracked lines went up, as did lines hit and lines missed, causing a dip in the coverage percentage. These changes are only in C files. Python numbers are identical. Checking a few C files, the main difference is https://codecov.io/gh/python-pillow/Pillow/src/28119dd68dc7915b7fd2d007f9e0742f1b6744fd/_imaging.c#L2944 I'm not quite sure what accounts for this. I'd guess it might be different versions of software used to instrument or measure or merge the code coverage in the new Dockerfiles, and that's changing the mixup. Whatever it is must account for the +0.92% in this commit, which removes one old dockerfile, and adds two newer ones. |
Just some notes about the differences between "native" and Docker builds: native:
Docker:
Docker, extracting commands:
Main thing is native is in-place, Docker is installed. |
Comparing Travis build logs between native 3.6 and Docker ubuntu-xenial-amd64: Docker can't find the coverage command in after_success.sh to do The main problem looks like installed builds don't collect C coverage properly. Just a few lines earlier is the C coverage results from
Installed:
I can reproduce this on a Mac. In-place v. installed: |
I was looking in the wrong place earlier for the C coverage files, so that's explained a bit. Ok, so in place is required for C coverage. I find that unfortunate, mainly in terms of testing and other uses for having the whole pile of docker images locally. Essentially, as the Pillow directory is mounted into the docker file, it restricts the docker image tree from ever having more than one concurrent run, as the incompatible .so files will stomp all over eachother. So, no parallel runs, debugging is harder, and all that entails. It's less of an issue on travis, but that's only because they're all running on independent machines anyway.
Everything in |
It may just be a matter of paths and configuration. In any case, having in-place on Travis doesn't prevent the possibility of installed for local debugging. |
And still not quite sure how this accounts for the coverage percentage change but let's see after switching Docker to in-place. |
Docker test environment updates